-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
V7 support for request batching #3370
V7 support for request batching #3370
Conversation
2270e3f
to
f7114f4
Compare
This is good for an initial review. It's still missing documentation but that may be best in a separate PR? Thoughts there? Usage examples can be seen in the tests for now. |
809c2bd
to
27e3ed4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From team code review 5/6/24
4fe1d57
to
3d343ca
Compare
Built-in types are not generics
- Implement some basic batching support for sync and async, running each request through request processing and response processing. - Achieve this through context manager that prevents request sending while batching is in progress and re-allows when batching is done. - Refactor batch middleware logic into Web3Middleware class - Fix up the sync batch method to work by using the ``w3._is_batching`` flag correctly. - Refactor the batching logic in the manager to be a bit more readable and more performant - even if ever so slightly. - Interestingly, using ``asyncio.gather()`` with x requests is still faster than batching the same amount of requests into a single batch request. More research into performance improvements is needed. - Iterate on performance improvement ideas for batching - Add basic tests for batch requests; minor typing & refactor
- When adding async requests to a batch for a batch request, handle the awaiting of extracting the request information in the manager instead of adding that overhead to the batch API. - Implement ``add_mapping()`` for batch requests
- Legacy websocket - IPC - Persistent connection providers (async_ipc, websocket)
- Move request batching to a higher level public API on the ``web3`` module; make the implementation methods on the manager private. - Create a proper ``AsyncWeb3ModuleTest`` class that can test differences between synchronous and asynchronous methods on the module that now exist. This gets rid of the parameterized tests that previously tested the static methods on both the ``Web3`` and the ``AsyncWeb3`` classes. - Tighten up typing related to batch requests; add unsupported methods for batching with tests
- Generalizaing name change: ``BatchRequestContextManager`` -> ``RequestBatcher`` since it is no longer just a context manager. - Add API to batcher for clearing (``clear``) and cancelling (``cancel``) the batch. - Add more tests around these changes.
- Only guarantee response `id` order for batch requests if every response has an `id` field. Otherwise, rely on the provider to return the responses in order and hope for the best. An `id` should really ever only be missing in very rare cases where the request object itself was not able to be parsed correctly. So, hopefully this proves to be robust enough for most use cases.
3d343ca
to
827788d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Feel free to take or leave any comments I left. I wonder if we make the RequestsInformation object a little more robust we could avoid all of the indexing - here for example. I was thinking maybe have RequestsInfo.method
, RequestsInfo.params
, RequestInfo.result_formatters
, RequestInfo.error_formatters
, etc. We could either pass around the object, or just whatever attributes each function needs. If that doesn't make sense, feel free to push back :)
- unpack response formatters for better readability
- unpack response formatters for better readability
8126f0f
to
56adab1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions for typing but lgtm!
) -> Union[ | ||
Callable[..., Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]], | ||
Callable[..., Coroutine[Any, Any, Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]]], | ||
]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth creating a type for RequestInformation
and use that inside the Callable[...]
s. There are a few places these are used. It could end up something like:
) -> Union[ | |
Callable[..., Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]], | |
Callable[..., Coroutine[Any, Any, Tuple[Tuple[RPCEndpoint, Any], Sequence[Any]]]], | |
]: | |
) -> Union[ | |
Callable[..., RequestInformation], | |
Callable[..., Coroutine[Any, Any, RequestInformation], | |
]: |
What was wrong?
Closes #832
How was it fixed?
Add batch request support to the
RequestManager
via a context manager. Expose this as a high-level API on theWeb3
/AsyncWeb3
class itself (w3.batch_requests()
).Todo:
EthereumTesterProvider
test caseCute Animal Picture